-
Notifications
You must be signed in to change notification settings - Fork 0
feat(Campaign): make context creation extensible via async overridable template method #315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: timowestnosto <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot act on feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces an extensible createContext
template method to the Campaign
component, enabling subclasses to customize context data before template rendering while maintaining full backward compatibility.
- Adds a new
createContext
method to theCampaign
class as an overridable extension point - Modifies the
loadCampaign
function to use the instance method instead of callinggetContext
directly - Includes comprehensive test coverage for the subclass override functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/components/Campaign/Campaign.ts | Adds the createContext template method and updates loadCampaign to use it |
test/components/Campaign.spec.tsx | Adds test coverage for subclass override behavior with custom context properties |
… test Co-authored-by: timowestnosto <[email protected]>
src/components/Campaign/Campaign.ts
Outdated
* } | ||
* ``` | ||
*/ | ||
createContext(raw: JSONResult): object { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what scenarios this will be useful? A custom campaign can only override context data?
Also, JSON recommendation results are of Nested objects. I am not sure if extending it at the top level is needed. Or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that I need to make this asynchronous, but the intended use cases are
- add utility functions to context
- use Shopify side product data instead
Co-authored-by: timowestnosto <[email protected]>
* class CustomCampaign extends Campaign { | ||
* async createContext(raw) { | ||
* const context = await super.createContext(raw); | ||
* return { ...context, customProperty: 'value' }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am still not understanding.
- add utility functions - Can you elaborate on what kind of utility functions we are planning to expose to, for e.g., merchant's template? Are we going to create these CustomCampaign elements case-by-case basis and expose utility functions specific to every case?
- Use Shopify product data - How/Why do we mix Shopify product data into Nosto's recommendation data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functions that could be useful are
- formatting functions
- event listener functions
We replace Nosto recommendation data with Shopify data for alignment
* | ||
* @example | ||
* ```typescript | ||
* class CustomCampaign extends Campaign { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the cases where we would extend the Campaign custom element? Wouldn't it make it complex trying to promote inheritance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see good alternatives for inheritance here, since registration of custom functionality into custom element classes is also not something that is available as a standard pattern
This PR introduces an extensible async
createContext
template method to theCampaign
component, allowing subclasses and future variations to customize context data before template rendering with support for asynchronous operations.Problem
Previously, the
Campaign
component calledgetContext(...)
directly during template rendering, making it impossible for subclasses to extend or modify the context without duplicating the entire render logic. This prevented scenarios like:Solution
Added an
async createContext(raw: JSONResult): Promise<object>
method to theCampaign
class that:await
instead of the directgetContext
usage inloadCampaign
Promise<object>
return type for better type safetyUsage Example
Changes
createContext
method toCampaign
class with detailed documentationloadCampaign
function to useawait
with the instance method instead of directgetContext
callPromise<object>
return type for better type safetyThe implementation is minimal and surgical - only one line changed in the rendering logic to add
await
and one async method added to the class.Fixes #314.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.